Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added CoordinateVector3 and used it in SphericalCoordinates #616

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Aug 13, 2024

🎉 New feature

Summary

As discussed in #597 (comment) , it seems that gz-math would benefit from having a CoordinateVector3 class that explicitly holds either spherical or metric coordinates.

This PR merges the fixes from #596 and #597 - it deprecates LOCAL2 frame and provides an unambiguous interface regarding angular units.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@peci1 peci1 changed the base branch from gz-math7 to main August 13, 2024 17:06
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Aug 13, 2024
@peci1 peci1 force-pushed the coordinate-vector branch 5 times, most recently from 7079db9 to 7185850 Compare August 16, 2024 02:14
@peci1
Copy link
Contributor Author

peci1 commented Aug 16, 2024

I'll be adding tests later (hopefully today). But please, comment on the proposed interface design.

@arjo129 arjo129 added beta Targeting beta release of upcoming collection and removed 🎵 harmonic Gazebo Harmonic labels Aug 16, 2024
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done a super thorough review, but have a minor knit about throwing exceptions.

include/gz/math/CoordinateVector3.hh Outdated Show resolved Hide resolved
include/gz/math/CoordinateVector3.hh Outdated Show resolved Hide resolved
@arjo129 arjo129 added the 🏛️ ionic Gazebo Ionic label Aug 16, 2024
…ad of throwing exceptions.

Signed-off-by: Martin Pecka <[email protected]>
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been through most of the code. This is looking pretty good. There is one minor knit I have about setting Nans without any logging.

src/ruby/SphericalCoordinates_TEST.rb Outdated Show resolved Hide resolved
src/ruby/SphericalCoordinates.i Show resolved Hide resolved
src/CoordinateVector3.cc Outdated Show resolved Hide resolved
src/CoordinateVector3.cc Show resolved Hide resolved
src/CoordinateVector3.cc Outdated Show resolved Hide resolved
src/SphericalCoordinates.cc Show resolved Hide resolved
arjo129 added a commit to gazebosim/gz-msgs that referenced this pull request Aug 19, 2024
With the introduction of gazebosim/gz-math#616.
We will be deprecating `LOCAL2` as a work around in favor or `LOCAL`. As
part of this change, its probably a good idea to update `gz-msgs` as
well.

Signed-off-by: Arjo Chakravarty <[email protected]>
@peci1
Copy link
Contributor Author

peci1 commented Aug 19, 2024

I've implemented what was requested in the last review. I also finished the test suite (with Ruby tests being more smoke tests than real tests).

Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for iterating on this PR.

@arjo129 arjo129 enabled auto-merge (squash) August 20, 2024 02:18
@arjo129 arjo129 merged commit 8dc652b into gazebosim:main Aug 20, 2024
9 checks passed
@azeey azeey added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Aug 20, 2024
azeey pushed a commit to gazebosim/gz-msgs that referenced this pull request Aug 22, 2024
* Deprecate LOCAL2 in `SphericalCoordinates`

With the introduction of gazebosim/gz-math#616.
We will be deprecating `LOCAL2` as a work around in favor or `LOCAL`. As
part of this change, its probably a good idea to update `gz-msgs` as
well.

Signed-off-by: Arjo Chakravarty <[email protected]>

* Fix deprecation warnings resulting from the deprecation of SphericalCoordinates enum field.

Signed-off-by: Martin Pecka <[email protected]>

---------

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Martin Pecka <[email protected]>
Co-authored-by: Arjo Chakravarty <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection Breaking change Breaks API, ABI or behavior. Must target unstable version. 🌱 garden Ignition Garden 🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants